fix: measure Cleanup's size estimates the way the cleanup actually works - #2103
Merged
Conversation
CleanupPreScanService sized both temp folders and the Recycle Bin with SearchOption.AllDirectories, which is wrong three ways: 1. It throws UnauthorizedAccessException out of MoveNext(), and the catch has to sit outside the foreach -- so one protected subfolder ended the whole walk and the headline reported whatever had been summed up to that point, silently low. 2. It follows reparse points. Deleting a junction puts one IN the Recycle Bin, so a single deleted junction could add every file it pointed at to the bin total. 3. It counted the extraction roots that the temp sweep has skipped since #2094, promising space Clean TEMP would correctly refuse to free. Both walks now go through TuneUpService.EnumerateFilesSkippingReparsePoints with the same two exclusions, which already has tests for the symlink, reparse-root and exclusion cases. The guard for that call shape read a hardcoded two-file list, so it could not see this caller at all -- the same weakness that let a third temp sweeper hide in a view-model for months. It now finds callers by looking for them. Its declaration filter matched the bare type name, which a call passing CancellationToken.None contains too, so both new calls would have been skipped as declarations; it now requires the type followed by a parameter name. Vacuity floor re-measured 5 -> 7. ICleanupPreScanService's contract offered "Unable to scan" as a temp label the method cannot produce; corrected to describe what it really returns. Closes #2098
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2098.
What was wrong
CleanupPreScanServicesized both temp folders and the Recycle Bin withDirectory.EnumerateFiles(path, "*", SearchOption.AllDirectories). Three defects from one call:AllDirectoriesthrowsUnauthorizedAccessExceptionout ofMoveNext(), and thecatchhas to sit outside theforeach— so the first folder Windows refuses ends the whole walk and the headline reports the partial sum, with nothing on screen to say so.SystemPaths.BundleExtractionRootandSystemPaths.OwnExtractionDirectory. The estimate did not, so it promised space Clean TEMP correctly refuses to free.Both walks now go through
TuneUpService.EnumerateFilesSkippingReparsePointswith the same two exclusions. That walker already carries tests for the directory-symlink, reparse-point-root and both-exclusion cases, so the pre-scan inherits proven behaviour rather than a second implementation.The guard could not see this caller
EveryTempTreeWalkerCall_PassesBothExtractionExclusionschecked walker arguments in a hardcoded["TuneUpService.cs", "DeepCleanupService.cs"]list — the same weakness that let a third temp sweeper hide inCleanupViewModelfor months. Two changes:SkippingReparsePoints(call. Only TuneUpService's walkers are reachable from outside, so that is exact.CancellationToken— whichCancellationToken.Nonecontains, so both new calls would have been skipped as declarations and the guard would have read them and checked nothing. It now requires the type followed by a parameter name.Vacuity floor re-measured 5 → 7 (five calls in the two sweepers, two in the pre-scan). Failure messages now name the file and the offending call.
Verification
Mutation proof, three cases, each restored byte-for-byte and re-hashed:
CleanupPreScanServicecall drops both exclusionsthis walker call passes neither or only one extraction exclusion — CleanupPreScanService.cs: EnumerateFilesSkippingReparsePoints(...)DeepCleanupServicecall drops both exclusionsDeepCleanupService.cs: EnumerateFiles(p, ct), so the loose-name coverage is intactDirectory.EnumerateFiles(AllDirectories)only 6 walker calls were matched across 3 file(s), so the floor catches a regression to the unsafe APIGreen at baseline and after restore. Regression sweep:
ArchitectureTests88 green (plus the harness-only author-header case for the throwaway runner),CleanupViewModelTests104 green,TuneUpServiceTests40 green. Builds 0 errors / 0 warnings,dotnet format --verify-no-changesclean on both projects, version consistency csproj 1.76.10 = CHANGELOG 1.76.10 = SECURITY 1.76.x.Not covered by a test: the abort-on-protected-folder behaviour itself. Reproducing it needs a directory the test host cannot read, which on an elevated CI runner an admin bypasses anyway — that would be a machine-dependent test, not a deterministic one. The mechanical pin is mutation 3: the population floor reddens if any call site goes back to
AllDirectories.Docs
ICleanupPreScanService's contract offered "Unable to scan" as a possible temp label, whichMeasureTempcannot produce — it skips per path and always reachesDescribe. Corrected to say what it really returns, including that an unreadable folder therefore reads as "Empty".